Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.46%. Comparing base (ee0da19) to head (80bd892).
✅ All tests successful. No failed tests found.

@@           Coverage Diff           @@
##             main    #4624   +/-   ##
=======================================
  Coverage   59.46%   59.46%           
=======================================
  Files         385      385           
  Lines       43039    43039           
  Branches    12383    12380    -3     
=======================================
+ Hits        25591    25594    +3     
+ Misses      17371    17369    -2     
+ Partials       77       76    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Claude Code Review

Summary

Since the last review the PR has been completely rewritten (all-new commits, rob/internal-shape-handles) to remove the handle-vs-ref ambiguity the earlier reviews flagged. The internal routing/filtering/identity layer is now uniformly keyed by an in-memory numeric shape_id minted by ShapeStatus, while shape_handle is preserved at every external/durable boundary (HTTP, storage/on-disk, the client Registry, the subquery tag-hash). The earlier mixed-type "optionality" between handle and id is gone — there is now exactly one boundary at each edge. The change is clean, well-commented, and the test suite is reported green (1833 tests, 0 failures); codecov reports all modified lines covered.

What's Working Well

• The structural concern from the previous round is resolved. Each module is now keyed by a single type: routing identity is id-only (ConsumerRegistry, EventRouter, DependencyLayers, Filter, SubqueryIndex, Materializer, RequestBatcher), and handle/id resolution happens once at each edge via small, clearly-commented helpers (consumer_pid_for_handle/2, dep_id_for_handle!/2, dependency_ids/2, id_for_handle/2). alco's "why the double naming of shape_ref/shape_id?" and "these maps/sets may now have two shapes of values" comments no longer apply — there is no shape_ref and no mixed-value map left.
• ShapeStatus id lifecycle is correct. mint_id/1 is an atomic :ets.update_counter on a :seq row keyed by an atom (can never collide with the positive-integer ids); add_shape/2 mints and inserts the {id, handle} reverse row; remove_shape/2 resolves the id before deleting the meta row and prunes the reverse row; reset/1 and table (re)creation clear all three tables.
• The refresh re-mint hazard (the long-open Critical) stays fixed. populate_shape_meta_table/2 reuses the existing id for any handle it already knows (so ids are stable across a refresh) and only mints for genuinely new handles, with an in-code comment explaining why re-minting would orphan the reverse mapping. refresh/1 then sweeps the reverse {id, handle} rows for handles still on the old generation before select_delete removes the stale meta rows — correct ordering, and the new 6-tuple match spec is consistent.
• Deletion races around the new lookups are handled gracefully and deliberately. start_materializers/3 aborts-and-purges on a missing inner id before starting the consumer; shape_cache.ex restore (start_consumer_for_id / restore_shape_and_dependencies) treats a vanished id as {:error, :no_shape}/{:error, handle}; shape_cleaner.ex resolves the id before remove_shape and nil-guards stop_consumer/3 and remove_from_shape_log_collector/2; the suspend path resolves the still-intact mapping. snapshotter.ex deliberately looks up the consumer via ConsumerRegistry.whereis(id) directly so a snapshot-failure report still lands after the handle/id mapping is gone — and that intent is documented in-code.
• Observability gap from the previous round is closed. list_shapes_with_ids/1 now Logger.warnings when it drops a shape with no id mapping (the only remaining concurrent-removal window), instead of silently dropping it. Log/telemetry sites that hold only an id use shape_handle_for_log/2, which returns "unknown, id: N" on a miss so callers can log unconditionally.
• Tests pin the load-bearing invariants (shape_status_test.exs): monotonic-id assignment, shape_handle_for_log fallback after removal, id stability across refresh/1, reverse-mapping prune when a handle is removed elsewhere, and list_shapes_with_ids. The changeset uses robacourt's suggested wording.

Issues Found

Critical (Must Fix)

None.

Important (Should Fix)

None.

Suggestions (Nice to Have)

1. Materializer.get_all_as_refs/2 is the one deletion-race site that raises a bare MatchError — materializer.ex:95-105

The line {:ok, dep_id} = ShapeStatus.id_for_handle(stack_id, shape_handle) hard-matches {:ok, _}. Every other handle/id resolution either threads :error through gracefully or raises with a descriptive message (dep_id_for_handle!/2 produces "missing shape_id for dependency handle ..."). Here a concurrently-removed dependency would surface as an opaque MatchError. It looks unreachable in practice (guarded by are_deps_filled/1, and I couldn't find a current caller of get_all_as_refs/2), so this is low priority — but for consistency consider reusing the bang-helper (clear message) or dropping the function if it's dead.

2. 45_000 magic timeout still inline in RequestBatcher.add_shape/4 — request_batcher.ex:64 (carried over, alco's earlier note)

The duplicate GenServer.call concern alco raised is gone (callers go through the defdelegate), but the bare 45_000 remains. A named module attribute (e.g. @add_shape_timeout) with a one-line rationale would document intent. Minor.

Issue Conformance

Still no linked issue. The (now very detailed) PR description matches the implemented scope, including the deliberate boundaries kept on shape_handle (HTTP/plug, all Storage.*/on-disk, the client Registry, PublicationManager/RelationTracker, and the byte-compatible subquery tag-hash). The four guarantees in the description (shape_id never on the wire, never persisted, tag-hash stays handle-keyed, the two registries stay distinct, no function takes both) are consistent with the diff I reviewed. Consider linking a tracking issue for the memory-reduction work.

Previous Review Status

Iteration 9 to 10 (full rewrite):

• Resolved — structural ambiguity (robacourt's review comment): internal layer is uniformly id-keyed; no mixed handle/id values remain.
• Resolved — alco's inline comments on request_batcher.ex: the shape_ref/shape_id double-naming and the "two shapes of values" maps are gone (only the 45_000 timeout note lingers, see Suggestion 2).
• Resolved — silent drop in list_shapes_with_ids/1: now logs a warning.
• Still standing — refresh id-stability + reverse-map prune: preserved in the rewrite and covered by tests.
• New (minor): Suggestion 1 (get_all_as_refs/2 hard match).

Nice rewrite — the boundary is now obvious from the code, the deletion-race handling is uniform and documented, and the tests pin the exact invariants the routing layer depends on.

Review iteration: 10 | 2026-06-22

I have only reviewed a part of it.

So far, the addition of optionality between shape_ref and shape_handle in multiple modules/functions looks like added complexity we would like to avoid. What's the heuristic I should use when reading the code to know whether a given function can end up taking a shape ref or a shape handle at runtime?

Great points. I've totally re-written it to remove ambiguity and to make the boundary clear.

@robacourt why have a handle -> id lookup at all. why not just replace shape handles with an integer (system_boot_time (microseconds) + atomic read and increment)? seems like we could lose the external handle vs internal id split and just use an integer everywhere (that's stringified in the url, de-stringified in the request params normalisation). we never use the fact that the handle is composed of the shape hash + timestamp. it's completely opaque so there's no need to keep that handle construction, we could just generate an integer (in a thread safe way). did you consider this? is there something I'm missing?

@robacourt why have a handle -> id lookup at all. why not just replace shape handles with an integer (system_boot_time (microseconds) + atomic read and increment)? seems like we could lose the external handle vs internal id split and just use an integer everywhere (that's stringified in the url, de-stringified in the request params normalisation). we never use the fact that the handle is composed of the shape hash + timestamp. it's completely opaque so there's no need to keep that handle construction, we could just generate an integer (in a thread safe way). did you consider this? is there something I'm missing?

I'd prefer this approach, but I'm struggling to find a way of fitting it into an 8 byte number (this PR manages to make it an 8 byte number by keeping it as an internal ID). You can only user 60bits before it starts using 24 bytes (boxed). You could squeeze boot time into 42 bits if it were milliseconds (rather than microseconds) and you used a custom epoch, that would give you ~139 years to play with. Then you'd have 18 bits left so if you went over 262K shapes you'd be into 24 byte numbers again. You also have the problem of two servers started in the same millisecond, but perhaps you could link it to when the database lock is acquired as no two servers can have that at the same time.

24 bytes would still smaller than our current shape handles which work out as 64 bytes once boxed. In reducing the memory footprint of the where clause filter you'd probably end up wanting a 8 byte ref to the 24 byte number which is what this PR was trying to avoid, but maybe that complexity is simpler than the complexity of this PR. What do you think @magnetised ?

@robacourt I don't think a 60 bit integer would limit us to ~300k shapes. I've thought about this a bit here and there and clauded this integer-only design:

Overview

Each ID is a 59-bit non-negative integer composed of a fixed boot prefix and a serial counter. Both prefix components are computed once at server boot; ID generation thereafter is a single atomic increment. Every ID is a single-word Erlang fixnum (no bignum allocation).

Bit layout

  [ 8 bits: boot-year ][ 51 bits: serial counter ]
    └ years since 2026 (0–255)      └ atomic, seeded at boot

Total width 59 bits, within the positive range of a 64-bit BEAM fixnum (max 2⁵⁹−1).

Boot procedure

At startup the server computes and freezes two values:

1. Year prefix = boot_year − 2026 (8 bits, range 0–255 → 2026–2281).
2. Counter seed = microseconds elapsed since the start of the boot year, written as the counter's initial value.

The seed spaces independent boots within the same year into different regions of the counter range, preserving rough chronological ordering across boots and reboots.

ID generation

id = (year_prefix << 51) | atomic_increment(counter)

A single atomic fetch-and-add per ID. No clock read after boot.

Behaviour past the boot year

The year field is the boot year, not the current wall-clock year — it is a fixed epoch tag, not a live clock. When a server runs past the calendar year boundary:

• The prefix stays at its boot value; the counter simply continues climbing in the same bucket.
• Nothing special happens at the boundary — no rollover, no reseed. IDs remain unique and monotonic.
• The only ceiling is exhaustion of the 51-bit counter (see capacity below), which takes decades at any realistic rate. A long-running server is unaffected.
• A reboot adopts the then-current year as its new prefix, moving to a fresh bucket and a fresh seed.

Consequently a server may run indefinitely across many calendar years on a single boot prefix; the prefix distinguishes boot epochs rather than tracking the wall clock.

Capacity

• Per boot bucket: 2⁵¹ = 2,251,799,813,685,248 (~2.25 quadrillion) IDs.
• Worst case — booting at the end of a year, seed ≈ a full year of µs (3.15×10¹³): ~2.22 quadrillion IDs still available. The bucket dwarfs a year of microseconds, so even an end-of-year boot consumes ~1.4% of the range.
• Aggregate across all 256 year prefixes: 2⁵⁹ = 5.76 × 10¹⁷ IDs.

Throughput

• Bucket-exhaustion ceiling: 2⁵¹ ÷ seconds-per-year ≈ 71.4 million IDs/sec sustained for a full year before a bucket is exhausted (~70× real time).
• Collision-avoidance ceiling: seeds are microsecond-spaced, so boots one second apart start 1,000,000 apart. A boot stays clear of a later same-year boot's region as long as it averages under 1 million IDs/sec.

Constants

  ┌────────────────────────────────┬───────────────────────────────┐
  │           Parameter            │             Value             │
  ├────────────────────────────────┼───────────────────────────────┤
  │ Epoch                          │ start of 2026                 │
  ├────────────────────────────────┼───────────────────────────────┤
  │ Total ID width                 │ 59 bits (positive fixnum)     │
  ├────────────────────────────────┼───────────────────────────────┤
  │ Year field                     │ 8 bits (2026–2281, 256 years) │
  ├────────────────────────────────┼───────────────────────────────┤
  │ Counter field                  │ 51 bits                       │
  ├────────────────────────────────┼───────────────────────────────┤
  │ IDs per boot bucket            │ 2,251,799,813,685,248         │
  ├────────────────────────────────┼───────────────────────────────┤
  │ Sustained rate (1-year bucket) │ ~71.4 M/sec                   │
  ├────────────────────────────────┼───────────────────────────────┤
  │ Safe rate (collision spacing)  │ ~1 M/sec                      │
  └────────────────────────────────┴───────────────────────────────┘

@robacourt I don't think a 60 bit integer would limit us to ~300k shapes. I've thought about this a bit here and there and clauded this integer-only design:

Love this! I'm still trying to work out how we can guarantee there's no collisions though. If we do have a collision, users could see data they're not supposed to see (currently a hash of the shape is included in the handle which prevents this). Collisions sound highly unlikely but there are a few scenarios they could happen:

1. two servers could start in the same microsecond - but they can't get the lock at the same microsecond, so maybe it could be the time we acquire the lock
2. two servers could acquire the lock at different times but think it's the same microsecond because their clocks are out of sync (I was thinking we could get the microsecond time from postgres once we have the lock, but there doesn't seem to be a way to do this that's immune to system clock jumps)
3. The above two scenarios don't have to be the same microsecond, they can be close and the counter from one can increment to the timestamp of the other

Closed in favour of #4643